Add config option to selectively delegate different operations to account threepid delegates #7611

babolivier · 2020-06-01T13:48:46Z

~~Add a registration_only configuration flag to account_threepid_delegates to allow only delegating 3PID registration, and letting Synapse handle password resets.~~

This PR adds a delegate_for config option to account_threepid_delegates which allows you to decide which services should be delegated to configured account threepid delegates, and which should remain as operations for the local Synapse to handle.

For context: account_threepid_delegates are intended to allow Synapse to ask a third-party process (usually an identity service) to handle sending emails/text messages for it. More information is available in the config option comment.

The reason this PR is so chunky is due to some refactoring around the complicated configuration handling of the account_threepid_delegates config option. This could probably be taken out into a separate PR, though it's a little fiddly.

I've tried my best to make this easy to review by commit, with explanations in each commit message.

~anoa

docs/sample_config.yaml

tests/rest/client/v2_alpha/test_register.py

babolivier · 2020-06-03T11:18:21Z

Hmm, I think I've identified a bug with this approach which is that we currently don't seem to store 3PIDs that have been registered with a delegated config, so password resets happening on Synapse might not work. I'll investigate this.

anoadragon453 · 2020-06-12T15:48:17Z

On the wording, we chose "bind" for binding a 3pid to an IS, and "add" for adding a 3pid to a user's account on a HS.

clokep · 2020-06-16T16:05:04Z

This is waiting for some feedback, @anoadragon453 said on #synapse-dev:matrix.org:

Ok, I am a bit stuck on this actually.

As part of #7611 we need to decide whether to contact an identity server to validate emails, or do it ourselves, based on whether the user is doing a password reset or registration. Unfortunately, both password reset and registration call these methods.

I eventually need to choose whether to use an IS or not here:

synapse/synapse/handlers/ui_auth/checkers.py

Lines 161 to 166 in a3f1156

if self.hs.config.threepid_behaviour_email == ThreepidBehaviour.REMOTE:

assert self.hs.config.account_threepid_delegate_email

threepid = await identity_handler.threepid_from_creds(

self.hs.config.account_threepid_delegate_email, threepid_creds

)

elif self.hs.config.threepid_behaviour_email == ThreepidBehaviour.LOCAL:

All of these methods are meant to be pretty generic, so threading something that's specific to the email checker is a bit tricky. Anyone have any thoughts on a simpler way to do this?

anoadragon453 · 2020-06-17T16:41:58Z

Hrmm, we have access to the session ID. I wonder if we could store metadata about a UI Auth session and then read it later.

clokep · 2020-06-17T17:02:22Z

@anoadragon453 Maybe the following are useful?

synapse/synapse/handlers/auth.py

Lines 452 to 482 in d0a43d4

    
               async def set_session_data(self, session_id: str, key: str, value: Any) -> None: 
        
                   """ 
        
                   Store a key-value pair into the sessions data associated with this 
        
                   request. This data is stored server-side and cannot be modified by 
        
                   the client. 
        
                   Args: 
        
                       session_id: The ID of this session as returned from check_auth 
        
                       key: The key to store the data under 
        
                       value: The data to store 
        
                   """ 
        
                   try: 
        
                       await self.store.set_ui_auth_session_data(session_id, key, value) 
        
                   except StoreError: 
        
                       raise SynapseError(400, "Unknown session ID: %s" % (session_id,)) 
        
               async def get_session_data( 
        
                   self, session_id: str, key: str, default: Optional[Any] = None 
        
               ) -> Any: 
        
                   """ 
        
                   Retrieve data stored with set_session_data 
        
                   Args: 
        
                       session_id: The ID of this session as returned from check_auth 
        
                       key: The key to store the data under 
        
                       default: Value to return if the key has not been set 
        
                   """ 
        
                   try: 
        
                       return await self.store.get_ui_auth_session_data(session_id, key, default) 
        
                   except StoreError: 
        
                       raise SynapseError(400, "Unknown session ID: %s" % (session_id,))

anoadragon453 · 2020-06-17T17:16:04Z

@clokep Ooo, generic, I love it, thank you!

clokep · 2020-06-17T18:00:23Z

@clokep Ooo, generic, I love it, thank you!

It does persist things into a database though so consider:

Whether it is OK to store unhashed
Whether it is JSON serializable

anoadragon453 · 2020-06-17T20:15:28Z

@clokep I'm envsioning just "register" and "password_reset", so should be fine 😁

anoadragon453 · 2020-06-18T10:59:10Z

As I will be checking this value during UI Auth, I need to classify the ui auth type while creating the session. As such, it doesn't look like I should use server_dict here (as we would always be inserting it into the database filled already).

I initially thought about using description to check instead, but that's intended to be a human-readable string that could change at any point, even across upgrades, so relying on that is a no-go.

My next thought is to add an operation column to ui_auth_sessions. This would contain a constant which identifies the purpose of the UI auth session, depending on where it is created. We can then check this later on.

I think this works well, though a slight niggle is backwards compatibility for all the existing sessions in that table. I could technically check the session description during a db migration and retroactively infer the operation of each one... however this would also be subject to description never changing.

We could also just wipe all ongoing UI auth sessions for an update.

What do you think?

clokep · 2020-06-18T11:03:19Z

My next thought is to add an operation column to ui_auth_sessions. This would contain a constant which identifies the purpose of the UI auth session, depending on where it is created. We can then check this later on.

Isn't this basically what the method + URI is used for?

In terms of modifying this table, I think we've generally considered it OK to be backwards incompatible. It is not ideal but the user impact is fairly low in this case.

anoadragon453 · 2020-06-18T11:10:00Z

Isn't this basically what the method + URI is used for?

@clokep hmm, this could work, but may be slightly iffy with URI's that include variables like:

synapse/synapse/rest/client/v2_alpha/devices.py

Line 98 in 7d28243

PATTERNS = client_patterns("/devices/(?P<device_id>[^/]*)$")

anoadragon453 · 2020-06-18T11:11:14Z

Also uh oh:

synapse/synapse/handlers/auth.py

Lines 305 to 306 in 5d64fef

    
           uri = request.uri.decode("utf-8") 
        
           method = request.uri.decode("utf-8")

clokep · 2020-06-18T11:25:05Z

Also uh oh:

synapse/synapse/handlers/auth.py

Lines 305 to 306 in 5d64fef

uri = request.uri.decode("utf-8")

method = request.uri.decode("utf-8")

Already fixed: #7689

And update sample_config.yaml.

So we initially had a variable called `threepid_behaviour_email` attached to the config object which was an instance of ThreepidBehaviour, the latter having three possible values - REMOTE, LOCAL or OFF. The rest of the codebase would basically use this to determine whether to try and do email stuff itself, or to delegate the operation to a third-party service (an identity server usually). Well, now that the sysadmin can choose whether to do this for adding a threepid vs. password reset, we now need two ThreepidBehaviour values. The code can then check either the one for adding a threepid, or the one for password reset, depending on what it's doing. This commit is breaking that up and also cleaning up the email template loading code such that templates are only loaded if necessary i.e. we'll be sending emails with that template. Oh, we also introduce ThreepidService, which is another enum like ThreepidBehaviour, but this one specifies which service you're validating (whether it's password reset or adding a threepid). I chose and enum again instead of a straight string as then we can easily avoid typos.

Now we start using the different `threepid_behaviour_email.*` variants for different account/registration operations.

Some additional refactoring for: * docstrings * lessening config option polling

We have a problem whereby during threepid (email) validation we need to know whether to handle the validation token ourselves or via an account threepid delegate. Earlier we could just check whether we were delegating email or not, but now that email can be either Synapse or a threepid delegate depending on which service (adding threepids, password reset) the validation is for, we need a way to find that out. So we store that information along with the validation session in the DB.

Store the type of service so that we can reference it during token validation.

The changes to identity.py are just threading the service through to `start_or_continue_validation_session`.

If a service is specified, check with our new `delegate_for` option for whether this should be handled by an account threepid delegate, or remotely. If there isn't a threepid session found with the details given by the client, assume that this is a remote session. Also modify the `is_enabled` method to check both `add_threepid` and `password_reset` services. If any of them are configured, then this checker is enabled. And again, refactor a little bit by pulling config option polling into the constructor.

tests/rest/client/v2_alpha/test_register.py

anoadragon453 · 2020-06-23T11:54:53Z

This is now ready for review.

@clokep I ended up attaching a service to threepid_validation_session in the DB instead of adding an operation to UIAuth sessions, as this classification was only relevant to the threepid checker of UIAuth anyways. Additionally, when users submit a threepid validation token, they don't submit the UIAuth session, so we couldn't check the auth session's operation anyways :)

anoadragon453 · 2020-07-02T10:03:09Z

We've decided to go with matrix-org/synapse-dinsic#51 as a solution to DINUM's problem instead. Additionally, this will hopefully allow for the deprecation of account_threepid_delegates.email instead of further complicating it.

babolivier requested a review from a team June 1, 2020 13:48

babolivier force-pushed the babolivier/3pid_only_delegate_reg branch from e21805f to c0b5c18 Compare June 1, 2020 14:02

erikjohnston reviewed Jun 1, 2020

View reviewed changes

docs/sample_config.yaml Outdated Show resolved Hide resolved

babolivier requested review from erikjohnston and a team and removed request for erikjohnston June 1, 2020 17:17

erikjohnston suggested changes Jun 3, 2020

View reviewed changes

tests/rest/client/v2_alpha/test_register.py Outdated Show resolved Hide resolved

babolivier force-pushed the babolivier/3pid_only_delegate_reg branch from a7bcc0c to 1b82724 Compare June 3, 2020 13:28

anoadragon453 self-requested a review June 12, 2020 11:52

anoadragon453 removed their request for review June 12, 2020 15:48

anoadragon453 self-assigned this Jun 12, 2020

anoadragon453 force-pushed the babolivier/3pid_only_delegate_reg branch 4 times, most recently from eb06fb0 to 91f1132 Compare June 22, 2020 17:46

anoadragon453 added 2 commits June 22, 2020 19:13

Add config option to selectively delegate 3pid validation

e0da3f6

And update sample_config.yaml.

Validate emails locally or remotely based on new ThreepidBehaviours

6448a3a

Now we start using the different `threepid_behaviour_email.*` variants for different account/registration operations.

anoadragon453 force-pushed the babolivier/3pid_only_delegate_reg branch 2 times, most recently from 7bc0549 to 6cedf93 Compare June 23, 2020 09:46

anoadragon453 added 5 commits June 23, 2020 12:42

Modify validate_threepid_session to check add_threepid.

74260e2

Some additional refactoring for: * docstrings * lessening config option polling

Store a ThreepidService alongside each validation session

3364cd0

Store the type of service so that we can reference it during token validation.

Specify the ThreepidService when creating a threepid validation session.

5af5b07

The changes to identity.py are just threading the service through to `start_or_continue_validation_session`.

anoadragon453 force-pushed the babolivier/3pid_only_delegate_reg branch from 6cedf93 to 38c755f Compare June 23, 2020 11:42

anoadragon453 reviewed Jun 23, 2020

View reviewed changes

tests/rest/client/v2_alpha/test_register.py Show resolved Hide resolved

anoadragon453 changed the title ~~Add config option to only delegate registration to an IS~~ Add config option to selectively delegate different operations to account threepid delegates Jun 23, 2020

anoadragon453 force-pushed the babolivier/3pid_only_delegate_reg branch from 38c755f to 667d075 Compare June 23, 2020 11:52

anoadragon453 requested a review from a team June 23, 2020 11:55

babolivier and others added 2 commits June 23, 2020 14:56

Add test cases for each possible configuration

0ba9328

Changelog

e446977

anoadragon453 force-pushed the babolivier/3pid_only_delegate_reg branch from 667d075 to e446977 Compare June 23, 2020 13:57

anoadragon453 requested review from clokep and removed request for a team and clokep June 23, 2020 14:00

anoadragon453 closed this Jul 2, 2020

babolivier deleted the babolivier/3pid_only_delegate_reg branch October 28, 2021 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config option to selectively delegate different operations to account threepid delegates #7611

Add config option to selectively delegate different operations to account threepid delegates #7611

babolivier commented Jun 1, 2020 •

edited by anoadragon453

Loading

babolivier commented Jun 3, 2020

anoadragon453 commented Jun 12, 2020

clokep commented Jun 16, 2020 •

edited by anoadragon453

Loading

anoadragon453 commented Jun 17, 2020

clokep commented Jun 17, 2020

anoadragon453 commented Jun 17, 2020

clokep commented Jun 17, 2020

anoadragon453 commented Jun 17, 2020 •

edited

Loading

anoadragon453 commented Jun 18, 2020

clokep commented Jun 18, 2020

anoadragon453 commented Jun 18, 2020 •

edited

Loading

anoadragon453 commented Jun 18, 2020 •

edited

Loading

clokep commented Jun 18, 2020

anoadragon453 commented Jun 23, 2020

anoadragon453 commented Jul 2, 2020

Add config option to selectively delegate different operations to account threepid delegates #7611

Add config option to selectively delegate different operations to account threepid delegates #7611

Conversation

babolivier commented Jun 1, 2020 • edited by anoadragon453 Loading

babolivier commented Jun 3, 2020

anoadragon453 commented Jun 12, 2020

clokep commented Jun 16, 2020 • edited by anoadragon453 Loading

anoadragon453 commented Jun 17, 2020

clokep commented Jun 17, 2020

anoadragon453 commented Jun 17, 2020

clokep commented Jun 17, 2020

anoadragon453 commented Jun 17, 2020 • edited Loading

anoadragon453 commented Jun 18, 2020

clokep commented Jun 18, 2020

anoadragon453 commented Jun 18, 2020 • edited Loading

anoadragon453 commented Jun 18, 2020 • edited Loading

clokep commented Jun 18, 2020

anoadragon453 commented Jun 23, 2020

anoadragon453 commented Jul 2, 2020

babolivier commented Jun 1, 2020 •

edited by anoadragon453

Loading

clokep commented Jun 16, 2020 •

edited by anoadragon453

Loading

anoadragon453 commented Jun 17, 2020 •

edited

Loading

anoadragon453 commented Jun 18, 2020 •

edited

Loading

anoadragon453 commented Jun 18, 2020 •

edited

Loading